Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new subsitute_subgraph() method to graph classes #823

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mtreinish
Copy link
Member

This commit adds a new method, substitute_subgraph(), to the PyGraph and PyDiGraph classes. It is used to replace a subgraph in the graph with an external graph.

This commit adds a new method, substitute_subgraph(), to the PyGraph and
PyDiGraph classes. It is used to replace a subgraph in the graph with an
external graph.
@coveralls
Copy link

coveralls commented Feb 24, 2023

Pull Request Test Coverage Report for Build 7573414576

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 95.874%

Totals Coverage Status
Change from base Build 7572561830: -0.09%
Covered Lines: 16173
Relevant Lines: 16869

💛 - Coveralls

@mtreinish mtreinish changed the title Add new subsitute_subgraph() graph classes Add new subsitute_subgraph() method to graph classes Mar 17, 2023
@mtreinish mtreinish added this to the 0.13.0 milestone May 10, 2023
@mtreinish mtreinish modified the milestones: 0.13.0, 0.14.0 Jun 1, 2023
@mtreinish mtreinish mentioned this pull request Jun 1, 2023
2 tasks
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, overall.

I spent some time trying to think of ways we could do the cycle check prior to making edits to the graph, if you think it's worth the effort. I believe we'd need to check the subgraph for all paths between incoming nodes to outgoing nodes, and then the original graph for paths going the other way between the corresponding IO nodes we're removing that would still be in tact after the replacement (no middle hops using nodes we're removing). We also might need special consideration for the case where input_node_map maps multiple indices from the original graph to the same new replacement node, since that would be very similar to contract_nodes, though I haven't thought it through.

I think we can just proceed with this as is in terms of the failure case leaving things in a bad state for now, and add preemptive checking for cycles in the future since that wouldn't be a breaking change.

src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
@@ -227,9 +227,10 @@ impl PyDiGraph {
p_index: NodeIndex,
c_index: NodeIndex,
edge: PyObject,
force: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name force for this parameter seems a bit misleading to me. In the context of this method's name, I would expect that force would mean to add the edge, even if it violates properties of the graph (e.g. introduces cycles). I'm also not a fan of us needing to pass a new parameter at all of the callsites for _add_edge.

Instead, what would you think about adding a new method called something like _error_if_would_cycle that takes the indices in question, and contains code moved from _add_edge which does the cycle check and error construction:

fn _error_if_would_cycle(&mut self, p_index: NodeIndex, c_index: NodeIndex) -> PyResult<()> {
    // Only check for a cycle (by running has_path_connecting) if
    // the new edge could potentially add a cycle
    let cycle_check_required = is_cycle_check_required(self, p_index, c_index);
    let state = Some(&mut self.cycle_state);
    if cycle_check_required
        && algo::has_path_connecting(&self.graph, c_index, p_index, state)
    {
        return Err(DAGWouldCycle::new_err("Adding an edge would cycle"));
    }
    Ok()
}

We could then call this from _add_edge if self.check_cycle is true, which preserves the current expectations of callers, and call it manually followed by a call to add_edge_no_cycle_check from methods like substitute_subgraph which offer cycle detection irrespective of whether or not self.check_cycle is enabled.

src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
src/digraph.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish modified the milestones: 0.14.0, 0.15.0 Jan 18, 2024
@mtreinish mtreinish removed this from the 0.15.0 milestone Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants